Unwrap sensitive values in error messages #42

smortex · 2023-02-24T22:46:04Z

When sensitive values are compared and do not match, the produce error message does not help for debugging:

  1) postgresql::server::role with Password Datatype Sensitive[String] has alter role for "test" user with password as ****
     Failure/Error:
       is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****')
         .with('command'     => sensitive('ALTER ROLE "test" ENCRYPTED PASSWORD \'new-pa$s\''),
               'sensitive'   => 'true',
               'unless'      => sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'"),
               'port'        => '5432')

       expected that the catalogue would contain Postgresql_psql[ALTER ROLE test ENCRYPTED PASSWORD ****] with command set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") but it is set to #<Sensitive [value redacted]>, and parameter unless set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") but it is set to #<Sensitive [value redacted]>
       Diff:
         <The diff is empty, are your objects producing identical `#inspect` output?>
     # ./spec/defines/server/role_spec.rb:56:in `block (3 levels) in <top (required)>'
     # /usr/home/romain/.gem/ruby/3.0/bin/rspec:25:in `load'
     # /usr/home/romain/.gem/ruby/3.0/bin/rspec:25:in `<main>'

With this change, the sensitive values are unwrapped and allow to spot the missing unwraps in unit tests:

  1) postgresql::server::role with Password Datatype Sensitive[String] has alter role for "test" user with password as ****
     Failure/Error:
       is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****')
         .with('command'     => sensitive('ALTER ROLE "test" ENCRYPTED PASSWORD \'new-pa$s\''),
               'sensitive'   => 'true',
               'unless'      => sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'"),
               'port'        => '5432')

       expected that the catalogue would contain Postgresql_psql[ALTER ROLE test ENCRYPTED PASSWORD ****] with command set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") but it is set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'Sensitive [value redacted]'"), and parameter unless set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") but it is set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'Sensitive [value redacted]'")
       Diff:

       @@ -1,4 +1,4 @@
       -Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'")
       +Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'Sensitive [value redacted]'")

       -Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'")
       +Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'Sensitive [value redacted]'")

smortex · 2023-02-24T23:40:04Z

For the record, this change is backward incompatible since matching any Sensitive value against the String "Sensitive [value redacted]" was previously passing and is now failing. It feels more logical to not introduce a magic string here IMHO, so I assumed this would be okay.

CLAassistant · 2023-04-19T21:48:54Z

All committers have signed the CLA.

LukasAud · 2023-07-13T14:39:17Z

Re-opening the PR due to age to re-kick CI. Want a fresh check on this before reviewing.

smortex · 2023-07-13T18:03:14Z

@LukasAud I rebased on top of master hopping that this would help. The workflow need approval however.

LukasAud · 2023-07-17T09:51:09Z

@smortex Sorry for the delay. Wasnt expecting the 'approve and run' function to be a blocker here. Tests are running now tho they are hitting rubocop rules.

smortex · 2023-07-17T19:56:05Z

Ooops, sorry about this, should have checked locally.

Rebased and fixed (rubocop and rspec seems happy on my computer). Thanks!

LukasAud · 2023-09-13T10:58:40Z

@smortex This is looking good. Im happy to drop an approval but I will hold of from merging as this is a backwards-incompatible and there is a few things we want to do before cutting a major release, including cutting a minor one beforehand.

Sorry for the delay but this will have to sit here for a bit longer.

When sensitive values are compared and do not match, the produce error message does not help for debugging: ``` 1) postgresql::server::role with Password Datatype Sensitive[String] has alter role for "test" user with password as **** Failure/Error: is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****') .with('command' => sensitive('ALTER ROLE "test" ENCRYPTED PASSWORD \'new-pa$s\''), 'sensitive' => 'true', 'unless' => sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'"), 'port' => '5432') expected that the catalogue would contain Postgresql_psql[ALTER ROLE test ENCRYPTED PASSWORD ****] with command set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") but it is set to #<Sensitive [value redacted]>, and parameter unless set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") but it is set to #<Sensitive [value redacted]> Diff: <The diff is empty, are your objects producing identical `#inspect` output?> # ./spec/defines/server/role_spec.rb:56:in `block (3 levels) in <top (required)>' # /usr/home/romain/.gem/ruby/3.0/bin/rspec:25:in `load' # /usr/home/romain/.gem/ruby/3.0/bin/rspec:25:in `<main>' ``` With this change, the sensitive values are unwrapped and allow to spot the missing unwraps in unit tests: ``` 1) postgresql::server::role with Password Datatype Sensitive[String] has alter role for "test" user with password as **** Failure/Error: is_expected.to contain_postgresql_psql('ALTER ROLE test ENCRYPTED PASSWORD ****') .with('command' => sensitive('ALTER ROLE "test" ENCRYPTED PASSWORD \'new-pa$s\''), 'sensitive' => 'true', 'unless' => sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'"), 'port' => '5432') expected that the catalogue would contain Postgresql_psql[ALTER ROLE test ENCRYPTED PASSWORD ****] with command set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") but it is set to Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'Sensitive [value redacted]'"), and parameter unless set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") but it is set to Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'Sensitive [value redacted]'") Diff: @@ -1,4 +1,4 @@ -Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'new-pa$s'") +Sensitive("ALTER ROLE \"test\" ENCRYPTED PASSWORD 'Sensitive [value redacted]'") -Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'new-pa$s'") +Sensitive("SELECT 1 FROM pg_shadow WHERE usename = 'test' AND passwd = 'Sensitive [value redacted]'") ```

smortex · 2023-10-13T19:48:23Z

👋 A new major release was cut a few days ago. I rebased my work on top of master to fix the conflicts.

Can we merge and release this?

ekohl

I just ran into this problem and can verify the patch helps a lot in debugging.

jordanbreen28

Amazing stuff @smortex - will save a lot of debugging pain for people in the future!

smortex marked this pull request as ready for review February 24, 2023 22:46

smortex requested a review from a team as a code owner February 24, 2023 22:46

smortex mentioned this pull request Feb 24, 2023

Fix wrong Sensitive handling for updating role passwords puppetlabs/puppetlabs-postgresql#1404

Merged

LukasAud added the backwards-incompatible label Jul 13, 2023

LukasAud closed this Jul 13, 2023

LukasAud reopened this Jul 13, 2023

LukasAud requested a review from bastelfreak as a code owner July 13, 2023 14:39

smortex force-pushed the unwrap-in-errors branch from c6b2ebf to 7b029c4 Compare July 13, 2023 18:01

smortex force-pushed the unwrap-in-errors branch from 7b029c4 to ce3611d Compare July 17, 2023 19:54

smortex force-pushed the unwrap-in-errors branch from ce3611d to b9dde53 Compare October 13, 2023 19:45

smortex requested a review from a team as a code owner October 13, 2023 19:45

bastelfreak approved these changes Oct 13, 2023

View reviewed changes

ekohl approved these changes Mar 8, 2024

View reviewed changes

jordanbreen28 approved these changes Mar 8, 2024

View reviewed changes

jordanbreen28 merged commit 6888827 into puppetlabs:main Mar 8, 2024

smortex deleted the unwrap-in-errors branch March 12, 2024 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unwrap sensitive values in error messages #42

Unwrap sensitive values in error messages #42

smortex commented Feb 24, 2023

smortex commented Feb 24, 2023

CLAassistant commented Apr 19, 2023 •

edited

Loading

LukasAud commented Jul 13, 2023

smortex commented Jul 13, 2023

LukasAud commented Jul 17, 2023

smortex commented Jul 17, 2023

LukasAud commented Sep 13, 2023

smortex commented Oct 13, 2023

ekohl left a comment

jordanbreen28 left a comment

Unwrap sensitive values in error messages #42

Unwrap sensitive values in error messages #42

Conversation

smortex commented Feb 24, 2023

smortex commented Feb 24, 2023

CLAassistant commented Apr 19, 2023 • edited Loading

LukasAud commented Jul 13, 2023

smortex commented Jul 13, 2023

LukasAud commented Jul 17, 2023

smortex commented Jul 17, 2023

LukasAud commented Sep 13, 2023

smortex commented Oct 13, 2023

ekohl left a comment

Choose a reason for hiding this comment

jordanbreen28 left a comment

Choose a reason for hiding this comment

CLAassistant commented Apr 19, 2023 •

edited

Loading